Skip to content

concat() support multiple string arguments#200

Merged
margarit-h merged 12 commits intointeg-concat-support-many-argsfrom
dev-concat-support-many-args
Jan 13, 2023
Merged

concat() support multiple string arguments#200
margarit-h merged 12 commits intointeg-concat-support-many-argsfrom
dev-concat-support-many-args

Conversation

@margarit-h
Copy link

@margarit-h margarit-h commented Jan 5, 2023 •
edited
Loading

Signed-off-by: Margarit Hakobyan margarit.hakobyan@improving.com

Description

These changes enable concat() string function to accept more than two arguments.
Usage: CONCAT(str1, str2, ...., str_n) adds two or more strings together.

Argument type: STRING, STRING, ...., STRING

Return type: STRING

Example::

os> source=people | eval `CONCAT('hello', 'world')` = CONCAT('hello', 'world'), `CONCAT('hello ', 'whole ', 'world', '!')` = CONCAT('hello ', 'whole ', 'world', '!') | fields `CONCAT('hello', 'world')`, `CONCAT('hello ', 'whole ', 'world', '!')`
fetched rows / total rows = 1/1
+----------------------------+--------------------------------------------+
| CONCAT('hello', 'world')   | CONCAT('hello ', 'whole ', 'world', '!')   |
|----------------------------+--------------------------------------------|
| helloworld                 | hello whole world!                         |
+----------------------------+--------------------------------------------+

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@margarit-h margarit-h marked this pull request as draft January 5, 2023 21:00
@codecov
Copy link

codecov bot commented Jan 5, 2023 •
edited
Loading

Codecov Report

Merging #200 (82acb60) into integ-concat-support-many-args (0e78fb6) will decrease coverage by 2.41%.
The diff coverage is 100.00%.

@@                         Coverage Diff                          @@
##             integ-concat-support-many-args     #200      +/-   ##
====================================================================
- Coverage                             98.35%   95.94%   -2.42%     
- Complexity                             3609     3624      +15     
====================================================================
  Files                                   344      355      +11     
  Lines                                  8946     9657     +711     
  Branches                                569      693     +124     
====================================================================
+ Hits                                   8799     9265     +466     
- Misses                                  142      334     +192     
- Partials                                  5       58      +53
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine 98.36% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...expression/function/BuiltinFunctionRepository.java 100.00% <100.00%> (ø)
...pensearch/sql/expression/function/FunctionDSL.java 100.00% <100.00%> (ø)
...l/expression/function/VarargsFunctionResolver.java 100.00% <100.00%> (ø)
...g/opensearch/sql/expression/text/TextFunction.java 100.00% <100.00%> (ø)
workbench/public/components/Main/main.tsx 53.00% <0.00%> (ø)
...ublic/components/QueryResults/QueryResultsBody.tsx 68.32% <0.00%> (ø)
workbench/public/components/SQLPage/SQLPage.tsx 100.00% <0.00%> (ø)
workbench/public/components/Header/Header.tsx 100.00% <0.00%> (ø)
workbench/public/application.tsx 0.00% <0.00%> (ø)
...ch/public/components/QueryResults/QueryResults.tsx 61.60% <0.00%> (ø)
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@margarit-h margarit-h requested a review from a team January 5, 2023 21:24
Yury-Fridlyand
Yury-Fridlyand reviewed Jan 5, 2023
View reviewed changes
Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to replace concat(one, two, three) with concat(one, concat(two, three))? In that case you don't need a vararg function.

acarbonetto
acarbonetto reviewed Jan 10, 2023
View reviewed changes
core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java
return funcBuilder;
}
if (functionResolverMap.get(functionName) instanceof VarargsFunctionResolver) {
return castArguments(sourceTypes, sourceTypes, funcBuilder);
Copy link

@acarbonetto acarbonetto Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourceTypes twice?

Copy link
Author

@margarit-h margarit-h Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As concat() has variable number of args and targetTypes will always be 0 (as the function signature is not fixed), failure will occur. So, in this case sourceTypes are passed instead of targetTypes to address that.

Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acarbonetto,
see #200 (comment)

Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a code comment is required to make things clear

@margarit-h margarit-h changed the title [DRAFT] Refactor concat() to support multiple string arguments [DRAFT] concat() support multiple string arguments Jan 10, 2023
@margarit-h margarit-h changed the title [DRAFT] concat() support multiple string arguments concat() support multiple string arguments Jan 10, 2023
@margarit-h margarit-h marked this pull request as ready for review January 10, 2023 19:35
Yury-Fridlyand
Yury-Fridlyand approved these changes Jan 11, 2023
View reviewed changes
Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java
return funcBuilder;
}
if (functionResolverMap.get(functionName) instanceof VarargsFunctionResolver) {
return castArguments(sourceTypes, sourceTypes, funcBuilder);
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a code comment is required to make things clear

@margarit-h
Refactor concat() to support multiple string arguments
8a9db78 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Allow not less than 2 args
042ef75 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Address a PR review comment
b9ca7d3 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Address a PR review comment
96f583d 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Refactoring
a93640c 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Added unit and integration tests
8e3cd32 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Add doctests
3d4e690 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Add correctness tests, fix checkstyle
1702ae7 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Edit docs
95167cc 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Add another unit test
c67b9f2 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Edited a javadoc
40ba368 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h
Address PR review feedback
82acb60 
Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@margarit-h margarit-h force-pushed the dev-concat-support-many-args branch from 8281bf8 to 82acb60 Compare January 11, 2023 19:30
@margarit-h margarit-h merged commit 26869fa into integ-concat-support-many-args Jan 13, 2023
margarit-h added a commit that referenced this pull request Jan 13, 2023
@margarit-h
Enable concat() string function to support multiple string arguments (
1f924f5 
#200)

* Refactor concat() to support multiple string arguments

Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Jan 13, 2023
Merged
6 tasks
GabeFernandez310 pushed a commit that referenced this pull request Jan 27, 2023
@margarit-h
Enable concat() string function to support multiple string arguments (
45fc371 
opensearch-project#1279)

* Enable `concat()` string function to support multiple string arguments (#200)

Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
matthewryanwells pushed a commit that referenced this pull request Feb 1, 2023
@opensearch-trigger-bot @margarit-h
Enable concat() string function to support multiple string arguments (
b62c55c 
opensearch-project#1279) (opensearch-project#1297)

* Enable `concat()` string function to support multiple string arguments (#200)

Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
(cherry picked from commit 45fc371)

Co-authored-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
@Yury-Fridlyand Yury-Fridlyand deleted the dev-concat-support-many-args branch March 10, 2023 19:46
andy-k-improving pushed a commit that referenced this pull request Nov 16, 2024
@dblock
Updated MAINTAINERS.md to match recommended opensearch-project format. (
47d7f63 
#200)

Signed-off-by: dblock <dblock@amazon.com>

Signed-off-by: dblock <dblock@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@acarbonetto acarbonetto acarbonetto left review comments

+1 more reviewer

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@margarit-h @Yury-Fridlyand @acarbonetto